refactor: move `CSigSharesManager` to `ActiveContext`, drop `CActiveMasternodeManager` from `CSigSharesManager` #6841

kwvg · 2025-09-08T23:51:05Z

Additional Information

Depends on refactor: drop fMasternodeMode global, create new context for masternode mode-only logic (ActiveContext), move spun-off signers and EHF signals handler #6828
Depends on chore: consolidate oft-repeated type definitions to {evo,llmq}/types.h #6839
This pull request does not address CQuorumManager or CDKGSessionManager as both complicate signer separation as they accommodate watch-only mode (source, source) and allow for LLMQ data recovery enablement (source), which makes them better suited for a separate PR.

In contrast, CSigSharesManager can be more-or-less wholesale moved to ActiveContext, the objective of this PR.
As part of isolating masternode-mode logic CSigningManager::AsyncSignIfMember(), which accepts CSigSharesManager as an argument, was moved to CSigSharesManager accepting CSigningManager as argument. This required exposing the recovered signatures DB to CSigningManager, which has been done with GetDb().
- As a result of this change and the move to ActiveContext, the quorum sign RPC will only work if masternode-mode is enabled
To allow dropping CActiveMasternodeManager from CSigningManager, we needed to drop a RelayRecoveredSig() call that is only done in masternode-mode (source). To do that, we rely on the NotifyRecoveredSig() signal that is invoked at the tail of CSigningManager::ProcessRecoveredSig() (source) by having ActiveNotificationInterface slot in, acting in response to the notification rather than before (i.e. existing behavior).

Breaking Changes

The quorum sign RPC will only work if masternode-mode is enabled.

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation (note: N/A)
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

github-actions · 2025-09-08T23:51:51Z

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

perf: improve islock / llmq signing latency #6896 - perf: improve islock / llmq signing latency
refactor: optimize AsyncSign by using std::move for quorum parameter #6906 - refactor: optimize AsyncSign by using std::move for quorum parameter
perf: improve signing latency by collecting lazy signatures under lock #6911 - perf: improve signing latency by collecting lazy signatures under lock

Please coordinate with the authors of these PRs to avoid merge conflicts.

github-actions · 2025-09-09T16:38:24Z

This pull request has conflicts, please rebase.

coderabbitai · 2025-10-22T17:45:45Z

Walkthrough

The patch refactors LLMQ signing and sigshare ownership: CSigningManager drops the active masternode dependency and the AsyncSignIfMember API; CSigSharesManager gains a richer, stateful interface (AsyncSignIfMember, NotifyRecoveredSig, ctor/destructor) and is moved from llmq::LLMQContext into ActiveContext as an owned member (shareman). Multiple constructors and Start/Stop/Interrupt signatures were updated (LLMQContext, CChainLocksHandler, CSigningManager, CSigSharesManager, ActiveContext). Call sites across chainlock, instantsend, ehf, net_processing, rpc/quorums, and masternode activation were changed to use active_ctx->shareman and the new APIs. A corresponding expected circular dependency test entry was added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title "refactor: move `CSigSharesManager` to `ActiveContext`, drop `CActiveMasternodeManager` from `CSigSharesManager`" directly and accurately describes the primary changes in the changeset. The raw summary confirms that CSigSharesManager is being restructured with lifecycle management moved to ActiveContext (evidenced by new Interrupt, Start, and Stop methods added to ActiveContext), and CActiveMasternodeManager dependency is being removed from both CSigningManager and CSigSharesManager. The title is specific, concise, and clearly communicates the main refactoring objective without ambiguity or unnecessary noise.
Description Check	✅ Passed	The PR description is directly related to the changeset and provides meaningful context about the refactoring. It explains the technical rationale behind moving CSigSharesManager to ActiveContext, details how CActiveMasternodeManager was removed by relocating AsyncSignIfMember to CSigSharesManager and using NotifyRecoveredSig() signals, identifies dependencies on prior PRs, acknowledges intentional deferral of other components (CQuorumManager and CDKGSessionManager), and clearly documents the breaking change that the quorum sign RPC now requires masternode-mode. The description appropriately supports understanding the changeset's scope and intent.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)

src/chainlock/chainlock.h (1)
8-19: Missing required headers in a public header

This header declares std::unique_ptr, std::thread, std::chrono::seconds, and uses assert(), but doesn’t include , , , . Add IWYU-complete includes to prevent fragile transitive dependencies.

Apply:
 #include <gsl/pointers.h>
 
+#include <chrono>
+#include <memory>
+#include <thread>
 #include <atomic>
 #include <map>
 #include <unordered_map>
+#include <cassert>
src/chainlock/chainlock.cpp (2)
65-82: Dangling reference: scheduler lambda captures isman by reference

Start(const CInstantSendManager& isman) schedules a lambda that outlives the function but captures [&] and uses isman. This is undefined behavior once Start() returns.

Capture a reference_wrapper by value (or store a member pointer) and capture [this, isman_ref]:
 void CChainLocksHandler::Start(const llmq::CInstantSendManager& isman)
 {
   if (auto signer = m_signer.load(std::memory_order_acquire); signer) {
     signer->Start();
   }
-  scheduler->scheduleEvery(
-      [&]() {
+  auto isman_ref = std::cref(isman);
+  scheduler->scheduleEvery(
+      [this, isman_ref]() {
         auto signer = m_signer.load(std::memory_order_acquire);
         CheckActiveState();
         EnforceBestChainLock();
         Cleanup();
         // regularly retry signing the current chaintip as it might have failed before due to missing islocks
         if (signer) {
-          signer->TrySignChainTip(isman);
+          signer->TrySignChainTip(isman_ref.get());
         }
       },
       std::chrono::seconds{5});
 }
219-233: Fix dangling references in scheduled lambdas: three instances require correction

The review comment correctly identifies a dangling reference issue in chainlock.cpp and the grep found two additional dangling reference problems in net_processing.cpp. All three capture function parameters or local variables by reference in scheduled lambdas that will outlive those references.

Locations requiring fixes:

src/chainlock/chainlock.cpp:219-233 — UpdatedBlockTip: Captures function parameter isman by reference via [&]

src/net_processing.cpp:1672 — ReattemptInitialBroadcast: Captures local variable delta by reference via [&]

src/net_processing.cpp:2021 — StartScheduledTasks: Captures local variable delta by reference via [&]

Apply the proposed fix pattern to all three: replace [&] with explicit captures of necessary references using std::cref() for reference types, or pass by value where appropriate.
src/llmq/signing.h (1)
8-20: IWYU: add and

Header declares std::unique_ptr and std::thread but doesn’t include their headers.
 #include <util/threadinterrupt.h>
 
 #include <gsl/pointers.h>
 
 #include <string_view>
 #include <unordered_map>
+#include <memory>
+#include <thread>
src/masternode/active/context.cpp (1)
1-1: Fix clang-format differences.

The pipeline indicates formatting issues in this file. Please run clang-format to fix the formatting differences.

[scratchpad_end] -->
#!/bin/bash
# Verify the exact formatting differences
git diff -U0 HEAD -- src/masternode/active/context.cpp | clang-format-diff -p1
src/llmq/signing.cpp (1)
714-715: Rename thread to avoid name collision with CSigSharesManager.

Duplicate "sigshares" thread names exist in both src/llmq/signing_shares.cpp:206 and src/llmq/signing.cpp:714. This hinders debugging and performance profiling. Rename to a distinct identifier:
-    workThread = std::thread(&util::TraceThread, "sigshares", [this, &peerman] { WorkThreadMain(peerman); });
+    workThread = std::thread(&util::TraceThread, "recsigs", [this, &peerman] { WorkThreadMain(peerman); });

🧹 Nitpick comments (9)

src/chainlock/chainlock.h (1)
66-67: Avoid chrono UDLs in headers

lastCleanupTime{0s} relies on std::chrono_literals being in scope for every TU including this header. Use explicit construction.
-    std::atomic<std::chrono::seconds> lastCleanupTime{0s};
+    std::atomic<std::chrono::seconds> lastCleanupTime{std::chrono::seconds{0}};
src/masternode/active/notificationinterface.cpp (1)
30-33: Add a defensive assert before dereferencing shareman.

Guard against accidental null in unusual init paths.
 void ActiveNotificationInterface::NotifyRecoveredSig(const std::shared_ptr<const llmq::CRecoveredSig>& sig)
 {
-    m_active_ctx.shareman->NotifyRecoveredSig(sig);
+    assert(m_active_ctx.shareman);
+    m_active_ctx.shareman->NotifyRecoveredSig(sig);
 }
src/rpc/quorums.cpp (1)
443-445: Consider checking active_ctx directly instead of mn_activeman.

Currently, the code checks node.mn_activeman to determine masternode mode, then uses CHECK_NONFATAL(node.active_ctx) when accessing shareman. While this works in practice (since active_ctx is created when mn_activeman exists), it would be more direct and safer to check active_ctx itself:
-    if (!node.mn_activeman) {
-        throw JSONRPCError(RPC_INTERNAL_ERROR, "Only available in masternode mode.");
-    }
+    if (!node.active_ctx) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Only available in masternode mode.");
+    }
Then the shareman access can use the already-verified pointer:
-    return CHECK_NONFATAL(node.active_ctx)->shareman->AsyncSignIfMember(...);
+    return node.active_ctx->shareman->AsyncSignIfMember(...);
This avoids relying on indirect implications and provides clearer error handling.

Also applies to: 467-467, 481-481
src/llmq/signing.cpp (1)
10-11: Drop unneeded include to reduce coupling.

#include <llmq/signing_shares.h> is not used in this TU. Removing it shrinks compile dependencies and helps avoid cycles.
-#include <llmq/signing_shares.h>
src/llmq/context.cpp (1)
51-57: Start order: register listeners before starting sigman’s worker.

To avoid a small race where early recovered-sig notifications are emitted before IS registers as a listener, start IS (and CL) first, then start sigman’s worker.
 void LLMQContext::Start(PeerManager& peerman)
 {
     qman->Start();
-    sigman->StartWorkerThread(peerman);
     clhandler->Start(*isman);
     isman->Start(peerman);
+    sigman->StartWorkerThread(peerman);
 }
If current ordering is intentional, confirm that no notifications can fire before IS registers.
src/llmq/signing_shares.h (1)
407-414: Consider const-correctness for m_chainstate.

CSigSharesManager reads from chainstate but doesn’t mutate it. Making it const CChainState& tightens API guarantees.
-    CChainState& m_chainstate;
+    const CChainState& m_chainstate;
src/llmq/signing_shares.cpp (3)

1206-1365: Message send batching LGTM; minor constant duplication note.

MAX_MSGS_* limits are duplicated across files; consider centralizing (optional).

1579-1601: Main loop order is sensible; small nit.

You already run RemoveBannedNodeStates() before work; consider placing Cleanup() before sleep_for to avoid extra 100ms latency after a busy cycle. Optional.

236-238: Use member m_sporkman instead of parameter to eliminate redundant argument.

CSigSharesManager stores m_sporkman as a member; refactor ProcessMessage to use it directly instead of accepting the same dependency as a parameter. The sporkman parameter is used only once in the function body (line 242).

Update locations:

src/llmq/signing_shares.h:432 — Remove const CSporkManager& sporkman parameter

src/llmq/signing_shares.cpp:236 — Remove parameter and replace sporkman.IsSporkActive(...) with m_sporkman.IsSporkActive(...)

src/net_processing.cpp:5380 — Update call: m_active_ctx->shareman->ProcessMessage(pfrom, msg_type, vRecv)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5ce1f0 and f12a3b7.

📒 Files selected for processing (19)

src/chainlock/chainlock.cpp (1 hunks)
src/chainlock/chainlock.h (1 hunks)
src/chainlock/signing.cpp (2 hunks)
src/init.cpp (3 hunks)
src/instantsend/signing.cpp (3 hunks)
src/llmq/context.cpp (2 hunks)
src/llmq/context.h (1 hunks)
src/llmq/ehf_signals.cpp (2 hunks)
src/llmq/signing.cpp (1 hunks)
src/llmq/signing.h (2 hunks)
src/llmq/signing_shares.cpp (37 hunks)
src/llmq/signing_shares.h (4 hunks)
src/masternode/active/context.cpp (3 hunks)
src/masternode/active/context.h (2 hunks)
src/masternode/active/notificationinterface.cpp (2 hunks)
src/masternode/active/notificationinterface.h (2 hunks)
src/net_processing.cpp (1 hunks)
src/rpc/quorums.cpp (5 hunks)
test/lint/lint-circular-dependencies.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/net_processing.cpp
src/chainlock/chainlock.cpp
src/llmq/signing.h
src/chainlock/chainlock.h
src/masternode/active/context.h
src/chainlock/signing.cpp
src/llmq/context.h
src/llmq/ehf_signals.cpp
src/masternode/active/notificationinterface.h
src/init.cpp
src/instantsend/signing.cpp
src/llmq/signing_shares.h
src/llmq/signing.cpp
src/rpc/quorums.cpp
src/llmq/context.cpp
src/masternode/active/context.cpp
src/llmq/signing_shares.cpp
src/masternode/active/notificationinterface.cpp

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

src/masternode/active/context.h
src/masternode/active/notificationinterface.h
src/masternode/active/context.cpp
src/masternode/active/notificationinterface.cpp

🧠 Learnings (2)

📚 Learning: 2025-01-02T21:50:00.967Z

Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.

Applied to files:

src/masternode/active/context.h
src/llmq/context.h

📚 Learning: 2025-10-02T18:29:54.756Z

Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.

Applied to files:

src/masternode/active/context.h
src/llmq/context.h
src/llmq/context.cpp

🧬 Code graph analysis (12)

src/llmq/signing.h (2)

src/llmq/signing.cpp (3)

CSigningManager (335-340)

CRecoveredSigsDb (30-33)

CRecoveredSigsDb (35-35)

src/llmq/signing_shares.cpp (2)

CSigSharesManager (183-195)

CSigSharesManager (197-197)

src/chainlock/chainlock.h (1)

src/chainlock/chainlock.cpp (2)

CChainLocksHandler (46-57)

CChainLocksHandler (59-63)

src/masternode/active/context.h (5)

src/llmq/context.cpp (6)

Interrupt (46-49)

Interrupt (46-46)

Start (51-57)

Start (51-51)

Stop (59-65)

Stop (59-59)

src/masternode/active/context.cpp (6)

Interrupt (49-52)

Interrupt (49-49)

Start (54-59)

Start (54-54)

Stop (61-66)

Stop (61-61)

src/init.cpp (2)

Interrupt (227-250)

Interrupt (227-227)

src/llmq/ehf_signals.h (2)

llmq (16-52)

CEHFSignalsHandler (21-51)

src/llmq/ehf_signals.cpp (2)

CEHFSignalsHandler (20-29)

CEHFSignalsHandler (31-34)

src/llmq/context.h (3)

src/llmq/context.cpp (2)

Start (51-57)

Start (51-51)

src/masternode/active/context.cpp (2)

Start (54-59)

Start (54-54)

src/instantsend/instantsend.cpp (2)

Start (76-88)

Start (76-76)

src/masternode/active/notificationinterface.h (3)

src/llmq/ehf_signals.h (1)

llmq (16-52)

src/llmq/signing_shares.cpp (2)

NotifyRecoveredSig (952-955)

NotifyRecoveredSig (952-952)

src/masternode/active/notificationinterface.cpp (2)

NotifyRecoveredSig (30-33)

NotifyRecoveredSig (30-30)

src/llmq/signing_shares.h (2)

src/llmq/signing_shares.cpp (24)

CSigSharesManager (183-195)

CSigSharesManager (197-197)

StartWorkerThread (199-207)

StartWorkerThread (199-199)

ProcessMessage (236-314)

ProcessMessage (236-237)

AsyncSignIfMember (872-950)

AsyncSignIfMember (872-874)

NotifyRecoveredSig (952-955)

NotifyRecoveredSig (952-952)

ProcessMessageSigSesAnn (316-346)

ProcessMessageSigSesAnn (316-316)

ProcessMessageSigSharesInv (354-389)

ProcessMessageSigSharesInv (354-354)

ProcessMessageGetSigShares (391-419)

ProcessMessageGetSigShares (391-391)

ProcessMessageBatchedSigShares (421-473)

ProcessMessageBatchedSigShares (421-421)

ProcessMessageSigShare (475-524)

ProcessMessageSigShare (475-475)

BanNode (1553-1577)

BanNode (1553-1553)

WorkThreadMain (1579-1601)

WorkThreadMain (1579-1579)

src/masternode/active/notificationinterface.cpp (2)

NotifyRecoveredSig (30-33)

NotifyRecoveredSig (30-30)

src/llmq/signing.cpp (1)

src/llmq/signing.h (1)

CSigningManager (160-244)

src/rpc/quorums.cpp (1)

src/llmq/signing_shares.cpp (2)

SelectMemberForRecovery (857-870)

SelectMemberForRecovery (857-857)

src/llmq/context.cpp (2)

src/instantsend/signing.cpp (4)

Start (49-52)

Start (49-49)

Stop (54-57)

Stop (54-54)

src/masternode/active/context.cpp (4)

Start (54-59)

Start (54-54)

Stop (61-66)

Stop (61-61)

src/masternode/active/context.cpp (5)

src/node/interfaces.cpp (2)

Interrupt (426-430)

Interrupt (431-436)

src/llmq/context.cpp (6)

Interrupt (46-49)

Interrupt (46-46)

Start (51-57)

Start (51-51)

Stop (59-65)

Stop (59-59)

src/init.cpp (2)

Interrupt (227-250)

Interrupt (227-227)

src/chainlock/signing.cpp (4)

Start (33-36)

Start (33-33)

Stop (38-41)

Stop (38-38)

src/instantsend/signing.cpp (4)

Start (49-52)

Start (49-49)

Stop (54-57)

Stop (54-54)

src/llmq/signing_shares.cpp (2)

src/llmq/signing.cpp (8)

StartWorkerThread (707-715)

StartWorkerThread (707-707)

WorkThreadMain (734-746)

WorkThreadMain (734-734)

ProcessMessage (393-433)

ProcessMessage (393-393)

Cleanup (635-648)

Cleanup (635-635)

src/net_processing.cpp (31)

ProcessMessage (3617-5411)

ProcessMessage (3617-3622)

pfrom (619-619)

pfrom (640-641)

pfrom (735-737)

pfrom (746-746)

pfrom (754-754)

pfrom (757-757)

pfrom (759-759)

pfrom (761-761)

pfrom (855-855)

pfrom (1037-1037)

inv (633-633)

inv (634-634)

inv (653-653)

inv (659-659)

inv (945-945)

inv (2326-2326)

id (685-685)

id (689-689)

SendMessages (5878-6478)

SendMessages (5878-5878)

LOCK (327-333)

LOCK (340-344)

LOCK (345-349)

pnode (639-639)

pnode (644-644)

pnode (721-721)

pnode (765-765)

pnode (835-835)

pnode (837-837)

src/masternode/active/notificationinterface.cpp (1)

src/llmq/signing_shares.cpp (2)

NotifyRecoveredSig (952-955)

NotifyRecoveredSig (952-952)

🪛 GitHub Actions: Clang Diff Format Check

src/llmq/signing_shares.h

[error] 1-1: Clang-format differences found in src/llmq/signing_shares.h. Run 'clang-format' or the diff script to fix formatting.

src/masternode/active/context.cpp

[error] 1-1: Clang-format differences found in src/masternode/active/context.cpp. Run 'clang-format' or the diff script to fix formatting.

🔇 Additional comments (29)

src/chainlock/chainlock.cpp (1)

46-56: Constructor changes align with header

Init list removal of sigman is consistent and safe.
src/instantsend/signing.cpp (3)
16-17: Include addition is appropriate

Directly using CSigSharesManager API warrants including signing_shares.h.

392-393: Consistent usage of AsyncSignIfMember

Call site aligns with new signature; relying on default fRetroactive=false is fine.

328-336: Parameter order and function signature verification complete—no issues found.

The function signature at src/llmq/signing_shares.h:443-447 confirms the parameter order is:
AsyncSignIfMember(Consensus::LLMQType llmqType, CSigningManager& sigman, const uint256& id,
                  const uint256& msgHash, const uint256& quorumHash = uint256(), bool allowReSign = false,
                  bool allowDiffMsgHashSigning = false)
Both calls in src/instantsend/signing.cpp align correctly:

Line 334: AsyncSignIfMember(llmqType, m_sigman, id, tx.GetHash(), {}, fRetroactive) maps parameters correctly (quorumHash={}, allowReSign=fRetroactive)

Line 392: AsyncSignIfMember(llmqType, m_sigman, id, tx.GetHash(), quorum->m_quorum_base_block_index->GetBlockHash()) uses defaults appropriately

All other usages across the codebase follow the same consistent parameter order.
src/chainlock/signing.cpp (2)

12-13: Include addition is appropriate

Using CSigSharesManager API requires including its header.

144-145: AsyncSignIfMember call updated correctly

Param order matches new interface. No further issues.
src/llmq/signing.h (1)
181-182: Constructor signature change successfully propagated; all call sites updated

The verification confirms the CSigningManager constructor signature change is complete. The only instantiation site at src/llmq/context.cpp:33 correctly uses the new 4-parameter signature without mn_activeman:
std::make_unique<llmq::CSigningManager>(chainman.ActiveChainstate(), *qman, unit_tests, wipe)
No stale 5-parameter call sites remain.
src/net_processing.cpp (1)

5378-5381: Move to ActiveContext::shareman looks good; please verify injection/lifetime.

Dropping the explicit PeerManager param means CSigSharesManager must already hold the correct PeerManager reference. Confirm:

ActiveContext builds shareman with this PeerManager instance.

shareman’s lifetime ≥ PeerManagerImpl usage.

No additional locks required beyond g_msgproc_mutex here.

src/masternode/active/notificationinterface.cpp (1)

9-9: Include is appropriate for calling CSigSharesManager APIs.

OK to include signing_shares.h here since the method call needs the full type.

test/lint/lint-circular-dependencies.py (1)

72-72: Circular dependency confirmed; refactoring is recommended but optional.

The cycle llmq/signing_shares → net_processing → masternode/active/context → llmq/signing_shares is real:

net_processing.cpp includes masternode/active/context.h

masternode/active/context.cpp includes llmq/signing_shares.h

llmq/signing_shares.cpp includes net_processing.h

Adding this to the expected circular dependencies list is acceptable if breaking the cycle is not feasible within this PR. However, the suggested refactoring approaches remain valid long-term improvements:

Extract a minimal interface header (e.g., net/peerman_iface.h) to decouple llmq/signing_shares from net_processing

Introduce an ISigSharesManager interface in masternode/active/context to eliminate the dependency chain

If this cycle was newly introduced by the PR's refactoring, consider whether these decoupling strategies could be applied now; otherwise, this is a good candidate for a follow-up refactoring effort.

src/masternode/active/notificationinterface.h (1)

10-10: LGTM!

The additions properly integrate recovered signature notifications into the active masternode notification interface. The forward declaration avoids unnecessary header dependencies, and the method signature follows the validation interface pattern correctly.

Also applies to: 14-16, 28-28

src/llmq/ehf_signals.cpp (1)

14-17: LGTM!

The include additions and AsyncSignIfMember parameter adjustment align with the refactor to move CSigSharesManager to ActiveContext and adjust dependency flow between signing managers.

Also applies to: 81-81

src/masternode/active/context.cpp (1)

29-37: LGTM!

The shareman initialization and lifecycle method implementations are correct. The refactor properly moves CSigSharesManager ownership to ActiveContext and wires it into the startup/shutdown sequence alongside the existing LLMQ DKG session manager.

Also applies to: 49-66

src/llmq/context.h (1)

44-44: LGTM!

The Start method signature change and removal of shareman member are consistent with the refactor goals. CSigSharesManager ownership has been successfully moved to ActiveContext, and LLMQContext.Start no longer requires CConnman since it doesn't manage shareman anymore.

Based on learnings.

src/init.cpp (1)

234-236: LGTM!

The lifecycle integration for ActiveContext follows the correct dependency order:

Startup: llmq_ctx → active_ctx (base first, dependents second)

Shutdown: active_ctx → llmq_ctx (dependents first, base second)

The Start method parameter changes align with the refactored ownership model.

Also applies to: 272-272, 2259-2260

src/masternode/active/context.h (1)

33-33: LGTM!

The interface design is sound:

Forward declaration minimizes dependencies

Public lifecycle methods follow established patterns

shareman is appropriately public for external access

cl_signer and is_signer are correctly private since they're managed through registration

The access levels reflect proper encapsulation.

Also applies to: 50-52, 61-61, 64-70

src/rpc/quorums.cpp (1)

31-31: LGTM!

The include addition enables access to ActiveContext, and the SelectMemberForRecovery change to use the static method is appropriate since it's a pure function called from a non-masternode-specific RPC command.

Also applies to: 750-750

src/llmq/signing.cpp (1)

335-340: Ctor change LGTM; dependency slimming achieved.

Dropping CActiveMasternodeManager here is clean and consistent with the PR goal. No functional risk spotted.

src/llmq/context.cpp (1)

33-36: Ctor wiring LGTM.

Updated CSigningManager and CChainLocksHandler ctor args align with the refactor.
src/llmq/signing_shares.h (3)
419-423: New constructor/dtor LGTM.

Explicit ctor with all dependencies, plus virtual dtor override, improves ownership clarity.

443-447: Verification confirms new API migration complete.

All 5 caller sites (rpc/quorums.cpp, ehf_signals.cpp, instantsend/signing.cpp, chainlock/signing.cpp) correctly use the new API pattern where AsyncSignIfMember is invoked on shareman with sigman passed as a parameter. No legacy calls using the old API remain in the codebase.

1-7: Apply clang-format to fix formatting violations in this file.

The file contains formatting issues that violate the project's .clang-format configuration, specifically:

Excessive blank lines (3+) at lines 104–108 and similar locations exceed the MaxEmptyLinesToKeep: 2 limit

Include organization may need adjustment

Run clang-format locally on src/llmq/signing_shares.h using:
clang-format -i src/llmq/signing_shares.h
Then commit the formatting changes to unblock CI.
src/llmq/signing_shares.cpp (7)

183-195: Ctor wiring LGTM.

Capturing deps by reference and resetting the interrupt is correct.

199-207: Worker start LGTM.

Parameterless StartWorkerThread() aligns with member-internalized deps.

639-705: Banning invalid lazy sigs is strict; confirm policy.

On invalid lazy sig deserialization, you ban immediately. If that’s intentional, all good; otherwise consider penalizing less harshly first.

Do we have prior art in net_processing for equivalent penalties?

725-774: Recovery flow LGTM.

On threshold reached, attempt recovery; single-member special-case is handled; sporadic verification guards against regressions.

Also applies to: 776-855

1150-1204: Announcement path LGTM; uses quorum node discovery once per quorum.

The memoization via quorumNodesMap avoids repeated GetMasternodeQuorumNodes calls.

1532-1577: Ban flow uses PeerManager::Misbehaving/IsBanned; good encapsulation.

Local state is cleaned up to re-request from others; matches resiliency goals.

1635-1655: Single-quorum signing still uses operator key; TODO acknowledged.

Noted; leaving as-is matches existing behavior.

src/chainlock/chainlock.h

src/llmq/signing_shares.h

src/llmq/signing.h

We initialize the masternode mode context later and as it's explicitly networking-heavy, we can move CConnman and PeerManager back into the ctor and trim down the arguments list a fair bit. Effectively reverts 82d1aed.

Giving write access of CRecoveredSigsDb to an external manager isn't great but we have to do this if we want to drop CActiveMasternodeManager from CSigSharesManager Review with `git log -p -n1 --color-moved=dimmed_zebra`.

As ProcessRecoveredSig() triggers a NotifyRecoveredSig() notification, we can stick the RelayRecoveredSig() into CSigSharesManager and have the relay request processed through the notification interface instead, lets us finally drop CActiveMasternodeManager from CSigSharesManager.

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/llmq/signing_shares.h (1)
20-30: Include NodeId definition.

This header uses NodeId in public types/APIs but doesn’t include net_types.h; include-what-you-use to avoid order-dependent builds.
 #include <uint256.h>
 
 #include <atomic>
 #include <limits>
 #include <memory>
 #include <optional>
 #include <string>
 #include <thread>
 #include <unordered_map>
 #include <utility>
 #include <vector>
 
+// Needed for NodeId
+#include <net_types.h>
src/rpc/quorums.cpp (1)
467-481: Verify active_ctx availability more explicitly.

Both calls use CHECK_NONFATAL(node.active_ctx) which will abort in debug builds but could lead to undefined behavior in release builds if active_ctx is null. Since masternode mode is already verified at line 443-445, and active_ctx should exist when mn_activeman exists, consider adding an explicit check:
 if (!node.mn_activeman) {
     throw JSONRPCError(RPC_INTERNAL_ERROR, "Only available in masternode mode.");
 }
+if (!node.active_ctx) {
+    throw JSONRPCError(RPC_INTERNAL_ERROR, "Active masternode context not available.");
+}
This provides better error handling and avoids relying on CHECK_NONFATAL in the success path.

🧹 Nitpick comments (1)

src/llmq/signing.h (1)
211-218: Minor redundancy: public GetDb() with friend declaration.

The code has both a public GetDb() method and a friend declaration for CSigSharesManager. The friend declaration is necessary to access the private ProcessRecoveredSig() method (line 214), but having GetDb() public makes the friend access to it redundant.

If GetDb() should only be accessible to CSigSharesManager, consider moving it to private/protected:
+private:
+    // Used by CSigSharesManager
+    CRecoveredSigsDb& GetDb() { return db; }
+    void ProcessRecoveredSig(const std::shared_ptr<const CRecoveredSig>& recoveredSig, PeerManager& peerman)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners);
+
+    // Needed for access to GetDb() and ProcessRecoveredSig()
+    friend class CSigSharesManager;
+
 public:
-    // Used by CSigSharesManager
-    CRecoveredSigsDb& GetDb() { return db; }
-    void ProcessRecoveredSig(const std::shared_ptr<const CRecoveredSig>& recoveredSig, PeerManager& peerman)
-        EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners);
-
-    // Needed for access to GetDb() and ProcessRecoveredSig()
-    friend class CSigSharesManager;
-
-public:
     // public interface
However, if other code legitimately needs direct database access, the current design is acceptable.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f12a3b7 and af9a15f.

📒 Files selected for processing (14)

src/chainlock/chainlock.h (2 hunks)
src/chainlock/signing.cpp (2 hunks)
src/instantsend/signing.cpp (3 hunks)
src/llmq/context.cpp (2 hunks)
src/llmq/ehf_signals.cpp (2 hunks)
src/llmq/signing.cpp (2 hunks)
src/llmq/signing.h (4 hunks)
src/llmq/signing_shares.cpp (37 hunks)
src/llmq/signing_shares.h (4 hunks)
src/masternode/active/context.cpp (3 hunks)
src/masternode/active/notificationinterface.cpp (2 hunks)
src/masternode/active/notificationinterface.h (2 hunks)
src/net_processing.cpp (1 hunks)
src/rpc/quorums.cpp (5 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

src/masternode/active/notificationinterface.cpp
src/net_processing.cpp
src/masternode/active/notificationinterface.h

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/llmq/signing_shares.h
src/llmq/ehf_signals.cpp
src/rpc/quorums.cpp
src/llmq/signing_shares.cpp
src/masternode/active/context.cpp
src/llmq/context.cpp
src/chainlock/signing.cpp
src/instantsend/signing.cpp
src/llmq/signing.cpp
src/llmq/signing.h
src/chainlock/chainlock.h

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

src/masternode/active/context.cpp

🧠 Learnings (1)

📚 Learning: 2025-10-02T18:29:54.756Z

Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.

Applied to files:

src/llmq/context.cpp

🧬 Code graph analysis (8)

src/llmq/signing_shares.h (1)

src/llmq/signing.cpp (10)

StartWorkerThread (707-715)

StartWorkerThread (707-707)

InterruptWorkerThread (729-732)

InterruptWorkerThread (729-729)

ProcessMessage (393-433)

ProcessMessage (393-393)

Cleanup (635-648)

Cleanup (635-635)

WorkThreadMain (734-746)

WorkThreadMain (734-734)

src/rpc/quorums.cpp (1)

src/llmq/signing_shares.cpp (2)

SelectMemberForRecovery (856-869)

SelectMemberForRecovery (856-856)

src/llmq/signing_shares.cpp (4)

src/llmq/signing.cpp (8)

StartWorkerThread (707-715)

StartWorkerThread (707-707)

WorkThreadMain (734-746)

WorkThreadMain (734-734)

ProcessMessage (393-433)

ProcessMessage (393-393)

Cleanup (635-648)

Cleanup (635-635)

src/llmq/quorums.cpp (4)

ProcessMessage (701-884)

ProcessMessage (701-701)

SelectQuorumForSigning (1204-1264)

SelectQuorumForSigning (1204-1205)

src/llmq/signing_shares.h (1)

MAX_MSGS_SIG_SHARES (377-493)

src/masternode/active/notificationinterface.cpp (2)

NotifyRecoveredSig (30-33)

NotifyRecoveredSig (30-30)

src/masternode/active/context.cpp (3)

src/llmq/context.cpp (6)

Interrupt (46-49)

Interrupt (46-46)

Start (51-57)

Start (51-51)

Stop (59-65)

Stop (59-59)

src/init.cpp (2)

Interrupt (227-250)

Interrupt (227-227)

src/instantsend/instantsend.cpp (4)

Start (76-88)

Start (76-76)

Stop (90-104)

Stop (90-90)

src/llmq/context.cpp (3)

src/instantsend/signing.cpp (4)

Start (49-52)

Start (49-49)

Stop (54-57)

Stop (54-54)

src/masternode/active/context.cpp (4)

Start (54-59)

Start (54-54)

Stop (61-66)

Stop (61-61)

src/chainlock/chainlock.cpp (4)

Start (65-82)

Start (65-65)

Stop (84-90)

Stop (84-84)

src/llmq/signing.cpp (1)

src/llmq/signing.h (1)

CSigningManager (162-243)

src/llmq/signing.h (2)

src/llmq/signing.cpp (5)

CSigningManager (335-340)

ProcessRecoveredSig (578-622)

ProcessRecoveredSig (578-578)

Cleanup (635-648)

Cleanup (635-635)

src/llmq/signing_shares.cpp (4)

CSigSharesManager (183-195)

CSigSharesManager (197-197)

Cleanup (1380-1514)

Cleanup (1380-1380)

src/chainlock/chainlock.h (1)

src/chainlock/chainlock.cpp (2)

CChainLocksHandler (46-57)

CChainLocksHandler (59-63)

🪛 GitHub Actions: Clang Diff Format Check

src/llmq/signing_shares.h

[error] 442-442: Clang format differences found. Run clang-format to fix formatting in this file.

src/masternode/active/context.cpp

[error] 46-46: Clang format differences found. Run clang-format to fix formatting in this file.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Build container / Build container

🔇 Additional comments (26)

src/chainlock/chainlock.h (2)
73-74: Constructor update looks correct.

Removal of CSigningManager& aligns with implementation and ownership shift.

70-70: Fix chrono literal in header (compile blocker).

0s requires std::chrono_literals; headers shouldn’t rely on using-directives. Initialize explicitly.
-    std::atomic<std::chrono::seconds> lastCleanupTime{0s};
+    std::atomic<std::chrono::seconds> lastCleanupTime{std::chrono::seconds{0}};
⛔ Skipped due to learnings
Learnt from: kwvg
PR: dashpay/dash#6815
File: src/chainlock/chainlock.h:68-69
Timestamp: 2025-08-13T16:10:32.972Z
Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. Chrono literals like 0s, 1min, etc. are acceptable and preferred over explicit constructors like std::chrono::seconds::zero().
Learnt from: kwvg
PR: dashpay/dash#6815
File: src/chainlock/chainlock.h:68-69
Timestamp: 2025-08-13T16:10:32.972Z
Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. The pattern using namespace std::chrono_literals; is used globally in src/util/time.h, and chrono literals like 0s are commonly used for atomic member initialization in headers (e.g., src/net.h). Chrono literals like 0s, 1min, etc. are preferred over explicit constructors.
src/llmq/signing_shares.h (5)
407-413: Objective mismatch: CActiveMasternodeManager still a hard dependency.

Member m_mn_activeman and ctor parameter persist, contrary to “drop CActiveMasternodeManager from CSigSharesManager”.

If the intent is to eliminate this dep, consider:

Injecting only required data (e.g., local ProTxHash) via an abstracted provider, or

Querying membership via qman and peerman (verified ProRegTx) without mn_activeman.
Would you like a follow-up patch to remove the dependency fully?

Also applies to: 419-423

443-447: AsyncSignIfMember moved here: ACK.

Signature matches usage sites (sigman passed in). No issues.

448-449: NotifyRecoveredSig hook: ACK.

Provides relay via peerman outside CSigningManager.

1-496: Developer must manually run clang-format to fix reported formatting differences.

The file src/llmq/signing_shares.h exists (496 lines as shown), but clang-format tool is unavailable in this verification environment. Since CI is reporting formatting failures, run clang-format locally using either:
contrib/devtools/clang-format.py -p src/llmq/signing_shares.h
# or
clang-format -i src/llmq/signing_shares.h
Then commit the formatting changes to resolve the CI pipeline failure.

432-432: ProcessMessage signature update verified complete.

All call sites have been correctly updated. The single call at src/net_processing.cpp:5380 uses the new 3-parameter signature without the sporkman reference, matching both the header declaration and implementation in src/llmq/signing_shares.{h,cpp}. No legacy 4-parameter calls remain.
src/chainlock/signing.cpp (1)

12-12: AsyncSignIfMember call switched to shareman: ACK.

Include added and call matches new API.

Also applies to: 144-145

src/llmq/signing.cpp (1)

333-341: CSigningManager dep cleanup: ACK.

Ctor no longer needs CActiveMasternodeManager.

Worker thread renamed to “recsigs”.

Relaying moved out; GetDb exposed as friend for shareman.

Also applies to: 714-715

src/instantsend/signing.cpp (1)

16-17: InstantSend signing paths use shareman.AsyncSignIfMember: ACK.

Matches refactor; no functional concerns spotted.

Also applies to: 334-335, 392-393

src/llmq/ehf_signals.cpp (1)

14-18: EHF signals: include updates and shareman.AsyncSignIfMember usage: ACK.

Consistent with new API.

Also applies to: 81-82
src/llmq/context.cpp (1)
33-37: I need to gather more information to fully verify the review comment. Let me search for call sites and related method changes:

I need to search the codebase directly to verify the lifecycle changes. Let me run more targeted queries:

Verify the Start() call sites in the codebase and check for Interrupt/Stop methods

Based on the script output provided, I can verify the key change mentioned in the review comment. The executed rg command confirmed that the Start method at line 51 in src/llmq/context.cpp now takes PeerManager& as its only parameter:
void LLMQContext::Start(PeerManager& peerman)
This matches the review comment's claim that "Start now takes PeerManager only."

However, I cannot fully verify the other claims in the review comment (shareman lifecycle moved to ActiveContext, sigman worker moved) without access to the full codebase context and previous versions of the code. The snippet provided shows the member initializations at lines 33-37 using std::make_unique, but without seeing the complete class definition, related files (ActiveContext, previous code state), and call sites, I cannot definitively confirm all the lifecycle changes mentioned.
src/masternode/active/context.cpp (2)

29-37: LGTM! Proper dependency injection and initialization order.

The shareman is correctly instantiated before being passed to dependent components (ehf_sighandler, cl_signer, is_signer), ensuring proper initialization order.

49-66: LGTM! Proper lifecycle management with LIFO ordering.

The Start/Stop/Interrupt methods follow correct LIFO ordering:

Start: DKG threads → shareman registration → shareman worker

Stop: shareman worker → shareman unregister → DKG threads

This ensures clean shutdown and matches the pattern used in LLMQContext.

Note: There's a clang-format issue on line 46 (empty line) flagged by CI that should be addressed.

src/rpc/quorums.cpp (2)

443-445: LGTM! Proper enforcement of masternode-mode requirement.

This check correctly enforces the documented breaking change that quorum sign RPC requires masternode-mode. The error message is clear and the check placement prevents execution of signing operations without the required context.

750-750: LGTM! Correct usage of static method.

The call correctly uses the static method CSigSharesManager::SelectMemberForRecovery, which is appropriate as this function doesn't require instance state.

src/llmq/signing.h (2)

23-25: LGTM! Necessary includes for standard library types.

The includes are justified:

<memory> for std::unique_ptr<CDBWrapper> (line 119)

<thread> for std::thread workThread (line 234)

183-183: LGTM! Constructor signature correctly removes active masternode dependency.

The removal of the CActiveMasternodeManager parameter aligns with the PR's objective to decouple CSigningManager from active masternode concerns. This responsibility is now handled by CSigSharesManager.

src/llmq/signing_shares.cpp (8)

183-197: LGTM! Clean dependency injection and RAII design.

The constructor properly injects all required dependencies as references, and initializes the worker interrupt. The default destructor is appropriate since cleanup is handled by explicit Stop() methods.

199-207: LGTM! Simplified thread start with member-based access.

The parameterless StartWorkerThread() correctly captures this to access member variables. Since all dependencies are stored as members initialized at construction, this is thread-safe.

236-313: LGTM! Consistent member-based access pattern.

The ProcessMessage method correctly uses direct member access (m_mn_activeman, m_sporkman) and consistently extracts node ID from the CNode reference. The early return for non-masternode mode (line 239) is appropriate.

871-949: LGTM! AsyncSignIfMember correctly relocated with proper dependencies.

The method has been successfully moved from CSigningManager to CSigSharesManager, now accepting CSigningManager& as a parameter. The implementation:

Correctly uses member access for active masternode and chainstate

Properly accesses the signing manager's database via friend access

Maintains the original voting and conflict checking logic

Ends with AsyncSign() to queue the signing operation

The method is thread-safe as it uses explicit locking (cs_pendingSigns via AsyncSign) and the database has its own internal locking.

951-954: LGTM! Clean delegation of recovery signature relay.

The NotifyRecoveredSig() method properly delegates to the peer manager for relaying. The Assert() ensures the signature pointer is valid before dereferencing, which is appropriate as this is called from internal code paths where the signature should always be valid.

775-854: LGTM! Proper signature recovery and processing flow.

The TryRecoverSig method correctly:

Handles single-member quorums (lines 792-809) and multi-member quorums (lines 811-851)

Validates recovered signatures periodically (line 842)

Delegates to sigman.ProcessRecoveredSig() with the peer manager reference

The sporadic self-verification (line 842-850) is a good practice to catch bugs without impacting performance.

1632-1700: LGTM! Consistent signature creation with member-based access.

The CreateSigShare method correctly:

Uses direct member access for active masternode (m_mn_activeman)

Handles both single-member (operator key) and multi-member (skShare) quorums

Maintains the existing TODO about single-member signing key (line 1651)

The conversion from pointer-based to member-based access is consistent and correct.

1148-1600: LGTM! Comprehensive and consistent refactor to member-based access.

The refactor systematically replaces parameter passing with member access throughout:

m_connman for connection management and messaging

m_peerman for peer management and banning

m_mn_activeman for masternode identity

m_sporkman for spork checks

All changes maintain thread safety as these members are initialized at construction and accessed appropriately. The messaging flow (PushMessage), banning logic (Misbehaving), and node iteration (ForEachNode) all work correctly with the member-based pattern.

PastaPastaPasta

overall LGTM, not super confident

UdjinM6

utACK af9a15f

kwvg added this to the 23 milestone Sep 8, 2025

github-actions bot added the needs rebase label Sep 9, 2025

kwvg modified the milestones: 23, 23.1 Oct 21, 2025

refactor: remove unused CSigningManager from CChainLocksHandler ctor

6563ade

kwvg force-pushed the active_ctx_p2 branch from 390e0b3 to 48b103a Compare October 22, 2025 11:25

github-actions bot removed the needs rebase label Oct 22, 2025

kwvg force-pushed the active_ctx_p2 branch from 48b103a to c99b4d3 Compare October 22, 2025 11:31

kwvg added 2 commits October 22, 2025 17:18

refactor: move CSigSharesManager to ActiveContext

74afa28

refactor: move CDKGSessionManager thread management to ActiveContext

f6b02b7

kwvg force-pushed the active_ctx_p2 branch from c99b4d3 to f12a3b7 Compare October 22, 2025 11:48

kwvg marked this pull request as ready for review October 22, 2025 17:33

coderabbitai bot reviewed Oct 22, 2025

View reviewed changes

src/chainlock/chainlock.h Show resolved Hide resolved

src/llmq/signing_shares.h Outdated Show resolved Hide resolved

src/llmq/signing.h Outdated Show resolved Hide resolved

kwvg added 4 commits October 22, 2025 23:44

refactor: adjust CSigSharesManager ctor arguments

489cf6e

We initialize the masternode mode context later and as it's explicitly networking-heavy, we can move CConnman and PeerManager back into the ctor and trim down the arguments list a fair bit. Effectively reverts 82d1aed.

refactor: move AsyncSignIfMember() to CSigSharesManager

0052fca

Giving write access of CRecoveredSigsDb to an external manager isn't great but we have to do this if we want to drop CActiveMasternodeManager from CSigSharesManager Review with `git log -p -n1 --color-moved=dimmed_zebra`.

lint: apply review suggestions

af9a15f

kwvg force-pushed the active_ctx_p2 branch from f12a3b7 to af9a15f Compare October 22, 2025 18:14

coderabbitai bot reviewed Oct 22, 2025

View reviewed changes

kwvg requested review from PastaPastaPasta, UdjinM6 and knst October 22, 2025 18:57

PastaPastaPasta approved these changes Oct 22, 2025

View reviewed changes

github-actions bot mentioned this pull request Oct 22, 2025

refactor: PreVerifyBatchedSigShares structured return and unit tests #6914

Open

5 tasks

UdjinM6 approved these changes Oct 22, 2025

View reviewed changes

PastaPastaPasta merged commit c44bc8c into dashpay:develop Oct 24, 2025
86 of 101 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: move `CSigSharesManager` to `ActiveContext`, drop `CActiveMasternodeManager` from `CSigSharesManager` #6841

refactor: move `CSigSharesManager` to `ActiveContext`, drop `CActiveMasternodeManager` from `CSigSharesManager` #6841

Uh oh!

kwvg commented Sep 8, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Sep 8, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Sep 9, 2025

Uh oh!

coderabbitai bot commented Oct 22, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

PastaPastaPasta left a comment

Uh oh!

UdjinM6 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor: move CSigSharesManager to ActiveContext, drop CActiveMasternodeManager from CSigSharesManager #6841

refactor: move CSigSharesManager to ActiveContext, drop CActiveMasternodeManager from CSigSharesManager #6841

Uh oh!

Conversation

kwvg commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional Information

Breaking Changes

Checklist

Uh oh!

github-actions bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Potential Merge Conflicts Detected

Uh oh!

github-actions bot commented Sep 9, 2025

Uh oh!

coderabbitai bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor: move `CSigSharesManager` to `ActiveContext`, drop `CActiveMasternodeManager` from `CSigSharesManager` #6841

refactor: move `CSigSharesManager` to `ActiveContext`, drop `CActiveMasternodeManager` from `CSigSharesManager` #6841

kwvg commented Sep 8, 2025 •

edited

Loading

github-actions bot commented Sep 8, 2025 •

edited

Loading

coderabbitai bot commented Oct 22, 2025 •

edited

Loading